Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Services e2e cleanup #14957

Merged
merged 3 commits into from
Oct 5, 2015
Merged

Services e2e cleanup #14957

merged 3 commits into from
Oct 5, 2015

Conversation

thockin
Copy link
Member

@thockin thockin commented Oct 2, 2015

Lots of misc cleanups to emit less noise during testing.

Flatten 3 tests which create load-balancers (slow) into one test.

Reduce e2e polling time in util.

Add a wait.PollImmediate() function (was a TODO) which dramatically speeds up e2e.

This test used to take 15+ minutes on my own e2e cluster and now takes less than 8 minutes.

@k8s-bot
Copy link

k8s-bot commented Oct 2, 2015

Unit, integration and GCE e2e test build/test passed for commit d6945c231b115f2b7f3c42f712278efc50328d95.

@k8s-github-robot k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 2, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XL

@gmarek
Copy link
Contributor

gmarek commented Oct 2, 2015

Thanks - those tests were causing a lot of pain!

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2015
@ncdc
Copy link
Member

ncdc commented Oct 2, 2015

@thockin I'd rather see poller modified to take a immediateCheck bool or something to that effect. PTAL at https://github.com/kubernetes/kubernetes/pull/14981/files. For example:

func poller(interval, timeout time.Duration, immediateCheck bool) WaitFunc {
    return WaitFunc(func() <-chan struct{} {
        ch := make(chan struct{})

        go func() {
            if immediateCheck {
                // send to the channel once immediately, rather than waiting for the first
                // interval to elapse
                ch <- struct{}{}
            }

And then you can implement PollImmediate by calling WaitFor(poller(interval, timeout, true), condition) whereas Poll would be WaitFor(poller(interval, timeout, false), condition).

Ultimately, I'd like us to end up with just 1 function, Poll, that always checks immediately. If a caller would prefer to wait, just add a time.Sleep before calling Poll. WDYT?

@ncdc
Copy link
Member

ncdc commented Oct 2, 2015

cc @smarterclayton

@thockin
Copy link
Member Author

thockin commented Oct 2, 2015

@ncdc the problem with putting it in the poller() func is that tests (which I added, ahem) inject their own need to replicate that logic. I think its cleaner to make that external to the test-provided WaitFunc. Agree with the end goal of one function that does the right thing.

I'd also like to eventually get rid of the "one last time" call on channel close - it's just a weird semantic, but apparently at least some code in the system depends on it (tests break).

@thockin
Copy link
Member Author

thockin commented Oct 2, 2015

I moved the wait change to #14996 - will rebase this when that is in.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 2, 2015

Unit, integration and GCE e2e test build/test passed for commit dba564f7a0ee1b987f0bbfd73732bc20aec76314.

@smarterclayton
Copy link
Contributor

This LGTM wrt service changes but I'm still in Central European time, haven't had coffee, and I'm reviewing this on a phone.

@thockin
Copy link
Member Author

thockin commented Oct 2, 2015

Rebased now that PollImmediate is in

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 3, 2015

Unit, integration and GCE e2e test build/test passed for commit 2f7552050bb37df319699259782005288734e84a.

@ghost ghost closed this Oct 5, 2015
@ghost ghost reopened this Oct 5, 2015
@ghost
Copy link

ghost commented Oct 5, 2015

@thockin needs another rebase, by the looks of things.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2015
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 5, 2015

Unit, integration and GCE e2e test build/test passed for commit ada7489.

a-robinson added a commit that referenced this pull request Oct 5, 2015
@a-robinson a-robinson merged commit 4856c7c into kubernetes:master Oct 5, 2015
piosz added a commit that referenced this pull request Nov 5, 2015
…-#14957-upstream-release-1.1

Automated cherry pick of #14957 upstream release 1.1
@thockin thockin deleted the services-e2e branch November 14, 2015 02:47
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
…ry-pick-of-#14957-upstream-release-1.1

Automated cherry pick of kubernetes#14957 upstream release 1.1
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
…ry-pick-of-#14957-upstream-release-1.1

Automated cherry pick of kubernetes#14957 upstream release 1.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants